Skip to content

feat(etl): weekly digest producer — @sigma/report extraction + Monday cron (#167A) - #80

Open
ydimitrof wants to merge 100 commits into
feat/ai-assistant-contractsfrom
feat/weekly-digest
Open

feat(etl): weekly digest producer — @sigma/report extraction + Monday cron (#167A)#80
ydimitrof wants to merge 100 commits into
feat/ai-assistant-contractsfrom
feat/weekly-digest

Conversation

@ydimitrof

@ydimitrof ydimitrof commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Implements #167A — Weekly Digest: Producer (pipeline · data · generation) from docs/implementation-plans/167-weekly-digest.md. This is the generation spine for „Седмицата в пари": every Monday the ETL worker writes a digest artifact to R2. Dev B consumes the StoredReport; this PR only produces it.

⚠️ Stacked PR — do not merge before #17

Base is feat/ai-assistant-contracts, not main. This is a hard dependency, not a convenience: the digest reuses verifier.ts, the ETL cron scaffold, and ADR-0007, none of which exist on main. Merge #17 first, then retarget this to main.

Migration is numbered 0004 on the assumption that #17 lands 0002/0003 first.

What's here

T1 — extract @sigma/report (~2d)

  • New packages/report — pure, worker-agnostic (no React, no cloudflare:*), so both the web worker and the ETL cron run the same emit→bind→validate→verify pipeline. It was previously trapped inside @sigma/web, which is why a cron couldn't reuse it.
  • Moved via git mv to preserve history: block schema, bindReport, validateEmitShape, sanitizeProse/Cell, findProseNumbers, asNumber, formatCell, verifier. @sigma/web re-exports from the new package, so the chat suites pass unchanged — that's the drift proof.
  • New persist.ts: StoredReport/Provenance, persistReport(bucket, key, stored, {immutable}), readStoredReport(bucket, key). R2 arrives as an injected R2Bucket — no binding names baked in.
  • New ISO-week util: isoWeekLabel(date), weekBounds(iso). Tested on the W52/W53/W01 boundaries.

T2 — DB layer

  • 0004_weekly_digests.sql: weekly_digests(iso_week PK, as_of, refreshed_at, status, total_eur) — an archive index only. The report payload stays in R2 as the single source of truth rather than being copied into D1.
  • packages/db/src/queries/weekly.ts: one function per indicator a–h. Money guarded with WHERE amount_eur IS NOT NULL, sector via substr(cpv_code,1,2), single-bid at bids_received=1 behind a ≥20 sample floor, top-10 carrying ids for links.
  • Reconciliation tripwire compares week SUM(amount_eur) against home_totals.value_eur and logs on drift. It deliberately compares value against value_eur and never equates counts — home_totals.contracts is a COUNT(*) over all rows, so it isn't the clean-amount count.

T3 — ETL cron

  • DIGEST_CRON = '0 7 * * 1' (Monday 07:00 UTC, after the 06:00 refresh), appended in the order cron-guard expects.
  • weekly-digest.ts pipeline: freshness anchor → GATE 1 settled-week (ADR-0007) → GATE 2 zero-row short-circuit → queries a–h → reconciliation → emit blocks → LLM narrative (BgGPT via AI Gateway) → bindReportfindProseNumbers gate → bounded regeneration → verifier strip → still invalid ⇒ AI-free fallback templatepersistReport(…) (no immutable — the object is overwritten in place on a re-issue, so immutable cache-control would be a stale-serve trap) → UPSERT → structured log.
  • wrangler.toml gains the AI and REPORTS bindings plus the kill-switch var; wrangler-render.mjs updated to match.

Both gates run before any query or LLM spend. Numbers are 100% SQL-bound — the model only writes connective narrative, and unsupported claims get stripped rather than rewritten. An unvalidated number is never persisted.

Tests

Written alongside, per the plan's TDD requirement:

  • Moved suites pass unchanged (proves the extraction didn't drift).
  • persist.ts validated against the golden fixture; ISO-week util on W52/W53/W01.
  • Weekly queries on a real-SQLite fixture with a seeded Mon–Sun week plus Sun 23:59 / Mon 00:00 boundary days; zero-row week returns empty; NULL amounts excluded.
  • Gate matrix: unsettled ⇒ skip; 0 contracts ⇒ asserts the LLM mock is not called and no put happens; invalid after N regens ⇒ fallback persisted; kill-switch off ⇒ no publish. cron-guard extended for DIGEST_CRON.

Notes for review

  • r2-report-object.fixture.json moved out of apps/web/.../fixtures/ — it's the frozen golden reference for both this ticket and Dev B's renderer.
  • Spec §6.1 says company → /companies/{ЕИК}; that's wrong for name-keyed bidders (name:<name>), so links route through entityHref/hrefForEntity rather than formatting ЕИК by hand.
  • Not in scope (Dev B / #167B): the /weeks/{ISO} SSR route, ReportBlockRenderer, ReportAiWatermark.

DiyanaDimitrova and others added 13 commits July 16, 2026 09:39
Read-side queries for the Weekly Digest producer: eight indicators (total
spend, volume, largest contract, single-bid rate with a 20-sample reporting
floor, week-over-week delta, top-10 contracts, sector breakdown, authority
breakdown) scoped by ISO 8601 week via strftime('%G-W%V', signed_at), plus a
local pure priorIsoWeek() helper and a log-only reconciliation check against
home_totals.value_eur. 0004_weekly_digests.sql adds the archive index table.
Consumer side of the weekly digest, built against the committed R2
artifact fixture (soft-dep on producer #167A):

- stored-report: interim StoredReport contract + readStoredReport /
  listStoredWeeks over R2 (moves to @sigma/report when #167A lands)
- ReportBlockRenderer: maps ResolvedBlock[] to reused components
  (totals/facts/table/timeseries) with entityHref deep-links; bar/flows
  render self-contained (generic blocks carry no entity ids); never
  dangerouslySetInnerHTML
- ReportAiWatermark, DigestFooter, WeeklyGhostBars (net-new ghost chart)
- routes: /weeks archive + /weeks/:iso detail, R2-only serve (no D1/LLM),
  404 for absent weeks, immutable cache for settled weeks
- weeks.css for digest-specific styling

Tests: 45 new (per-block golden renders, loader 404 + no-D1 guard,
contract drift guard, full-page golden, AI-free fallback). Full web
suite green (404 tests), typecheck clean.
Move the pure report pipeline (report-schema, emit-report-schema,
verifier, temporal, describe-schema, the StoredReport contract) out
of apps/web into a new @sigma/report workspace package so apps/etl
can build weekly digests without depending on @sigma/web. Barrel
shims at every old apps/web path (`export * from '@sigma/report'`)
keep the ~30 existing import sites resolving unchanged; the moved
test suites are the drift proof and pass unmodified in their new
home.
Extract a pure buildStoredReport/persistReport/readStoredReport API
from agent.ts's chat-coupled persistReport, so both the chat lane and
the ETL weekly-digest producer can build/persist the same StoredReport
shape without a ToolContext. agent.ts now wraps the shared builder,
keeping chat behavior (random id, report/{id}.json key, error
swallowing) unchanged.

Add packages/report/src/iso-week.ts (priorIsoWeek) for the Monday
cron's Mon-Sun ISO-week resolution, distinct from temporal.ts's
question-parsing half-open bounds. Covers the W52/W53/W01 year
boundaries.
Superseded by assistant-contract/fixtures/stored-report.sample.json (the one
wired into fixtures.test.ts); had zero code references after the @sigma/report
extraction.
Adds the wrangler bindings and dependencies the weekly digest producer
needs: the REPORTS R2 bucket (fixed bucket_name, matching apps/web's
committed binding — lower risk than teaching wrangler-render.mjs's TOML
path to rename R2 per-env, which risked the two workers drifting to
different bucket names), the AI Gateway vars (AI_GATEWAY_BASE_URL,
ASSISTANT_MODEL) and the DIGEST_ENABLED kill switch (committed "false",
fail-dark like ASSISTANT_ENABLED). Adds DIGEST_CRON ('0 7 * * 1') in
lockstep with wrangler.toml's crons array and the cron-guard test, and
the @ai-sdk/openai, ai, @sigma/report and @sigma/db workspace deps.
Adds generateWeeklyDigest, dispatched from scheduled() on DIGEST_CRON
behind the DIGEST_ENABLED kill switch. Anchors on home_totals.as_of to
resolve the prior full ISO week (@sigma/report's priorIsoWeek), gates
on settlement (ADR-0007 posture: as_of >= the week's Sunday) and on a
zero-contracts short-circuit (no LLM call, no R2 write) before running
the @sigma/db weekly queries a-h and the reconciliation tripwire.

Builds the report's data blocks (totals/table/bar) deterministically
from the query results and asks a single injected GenerateFn for a
number-free lead narrative (BgGPT via the Cloudflare AI Gateway, wired
the same way apps/web/app/lib/assistant/agent.ts's buildModel does —
fail-closed without AI_GATEWAY_BASE_URL). bindReport's material-number
gate rejects a narrative that leaks a figure; up to one retry, then an
AI-free fallback (data blocks only, no model prose) is bound and used
instead. verifyReport's role-4 pass strips any unsupported claim before
persisting. Sanity gates (total >= 0, largest <= total, plausible WoW
delta) block publish outright rather than persist an unvalidated
number. Persists an immutable StoredReport to weeks/{iso}.json and
UPSERTs weekly_digests, marking a re-run over an existing week
"коригирано".

Tests cover the full gate matrix with a fake D1 + R2 bucket and a mock
GenerateFn: unsettled week, missing as_of, zero contracts (asserting
neither generate nor bucket.put are called), sanity-gate failures, the
valid path, reissue-over-existing, narrative rejected/thrown on every
attempt (asserting the AI-free fallback carries no unbound prose
number), and the DIGEST_ENABLED kill switch's fail-dark truth table.
An LLM response that trims to empty silently continued to the next attempt,
reading in logs as if the narrative step never ran. Emit a distinct
etl_digest_narrative_empty event (mirroring the throw/reject branches) so the
fallback is observable, with a test asserting it fires once per attempt and the
AI-free fallback still persists.
@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown

🌐 Preview deployedhttps://sigma-pr-80.midt-platforms.workers.dev

Worker sigma-pr-80 · shares the dev D1 (read-only). Updates on each push; removed when this PR closes.

getWeeklyCounts returned only COUNT(*) — the raw activity volume, which
includes rows the week's SUM(amount_eur) excludes. Callers pairing that
count with the money total would present two different row sets as one
KPI set, against precompute.sql's COUNT/SUM CONSISTENCY rule.

Add contractsWithAmount (the count behind getWeeklyTotal). contracts is
left as-is: it is the honest volume metric, and the digest's zero-row
publish gate keys on it.
The totals strip rendered „Договори" (COUNT(*), every signed row) right
next to „Обща стойност" (SUM over amount_eur IS NOT NULL). The two cover
different row sets, so dividing one by the other yielded a wrong average
contract value — on a seeded 30-clean + 1-NULL week, 44,758 instead of
46,250.

Bind the clean-basis count in the strip and expose it as R1's
contracts_with_amount. The raw volume stays in R1 for the zero-row gate.
status/model keyed on narrativeMd — whether the narrative BOUND — not on
whether it survived the verifier. A verifier that strips every claim (it
fails closed on malformed verdicts) leaves an artifact with no model
prose at all, yet it was stored as status='ok' with provenance.model
naming the model. The archive index reads status, so a numbers-only
digest advertised itself as model-authored.

Key both on the text block surviving verification. A partial strip still
leaves prose and correctly stays 'ok'.
DiyanaDimitrova and others added 10 commits July 16, 2026 15:02
Integrates the landed digest/report foundation (@sigma/report package with
persist + iso-week utils, weekly-digest DB queries + migration 0004, ETL
cron/generator, canonical ReportBlockRenderer/ReportAiWatermark, /reports
route) and reconciles the interim consumer scaffolding onto the canonical APIs:

- drop interim apps/web/app/lib/assistant/stored-report.{ts,test.ts} —
  StoredReport + readStoredReport now come from @sigma/report
- move digest-only archive helpers to apps/web/app/lib/weeks.ts
  (isoWeekKey / isValidIsoWeek / listStoredWeeks)
- weeks.$iso: read via @sigma/report, strip provenance from the client payload,
  render <ReportBlockRenderer blocks> + static <ReportAiWatermark>, guard the
  optional REPORTS binding
- keep both /weeks and /reports routes; keep weeks.css alongside assistant.css
- remove the now-invalid interim component tests (theirs are canonical)

Verified: @sigma/web 1164 tests pass, typecheck clean.
…links (#167B)

Closes the plan-audit MVP gaps on the digest:

- §3.4 daily ghost-bars — new getWeeklyDailySpend (this week + prior, 7 Mon–Sun
  zero-filled slots) in @sigma/db; additive `weekbars` block in @sigma/report
  (schema + bindReport + validateEmitShape); the generator emits it (R6/R7) and
  ReportBlockRenderer renders it via WeeklyGhostBars — the net-new chart is now
  wired end-to-end (was built but orphaned).
- §3.8 competition — a concentration bar (single-bid vs multi-bid counts, R8),
  gated on the reporting-sample floor.
- §3.10 „Разгледай сам" — code-generated deep links (contracts/authorities/
  companies/flows) on /weeks/:iso.

Tests: @sigma/db 223, @sigma/report 222, @sigma/etl 46, @sigma/web 1166 — all
pass; typecheck clean across all packages.

Remaining (documented): §3.8 stacked-procedure lane (needs a weekly
procedure-type query + a stacked block type); week-scoped list filtering for the
explore links (needs a `?week=` loader param).
Non-blocking review follow-ups (PR #81 review by @ydimitrof):

1. weekly-digest.ts: guard the R8 (single-bid) snapshot push under the SAME
   rate!==null condition that gates the competition bar, so the persisted
   snapshot never carries a dead result no block references.
2. weekly.ts: document the date-alignment invariant in daySpendFor —
   substr(signed_at,1,10) and weekDates()'s UTC slots read the same stored
   UTC date, the basis WEEK_FILTER's strftime already relies on.
3. WeeklyGhostBars: comment the index-pairing invariant (both series are the
   fixed 7 Mon..Sun slots; do not reuse with unaligned series).
4. Record the week-scoped deep-link (?week=) follow-up in docs/tickets/167b
   and point DigestExplore's NOTE at it.

No behaviour change beyond the R8 guard. Tests + typecheck green across
@sigma/db, @sigma/etl, @sigma/web.
…ropagate (#81 M1)

Strict-review M1: /weeks/:iso returned `Cache-Control: immutable` (s-maxage 1y),
but the producer re-issues a corrected digest in place at the same
`weeks/{ISO}.json` key (status „коригирано", spec §10.4). `immutable` tells the
edge/browser never to revalidate, so a correction would not reach readers for up
to a year. Switch to `s-maxage=1d, stale-while-revalidate=7d` — near-static edge
performance while a late-data correction still propagates within a day. Added a
headers() regression test asserting the bounded policy (not immutable).

When StoredReport gains a settled/refreshedAt signal, a truly-settled week can
return to immutable.
… workers-types dep

Resolves two review findings on PR #80:

- MINOR (review 5078506825): the base seed only paired value_flag='value_suspect'
  with a NULL amount_eur, so „summed in total, excluded from largest/top" held by
  construction. Add a suspect row carrying the week's HIGHEST non-NULL amount_eur and
  assert it lands in getWeeklyTotal (flag-independent basis) yet is filtered from
  getWeeklyLargestContract / getWeeklyTopContracts (value_flag='ok' guard).

- MEDIUM (strict review 5094463521): @sigma/report used R2Bucket/ExecutionContext
  ambient types without declaring @cloudflare/workers-types; add it as a devDependency
  so the package type-resolves in isolation.
@lyubomir-bozhinov

Copy link
Copy Markdown
Owner

The new commit's SUSPECT_HI test is exactly the discriminating case the base seed couldn't cover: a value_suspect row with a real (non-NULL) amount_eur=9000 that's the week's highest. Verified the three assertions against the actual query guards — total=12000 includes it (rollup basis is amount_eur IS NOT NULL, flag-independent), while largest→SUN(2000) and top→[SUN,MON] both exclude it (the value_flag='ok' guard). That's the correct sigma value_flag contract — money rollups sum regardless of flag, per-contract price surfaces guard 'ok' — now proven with data rather than by-construction. Good addition.

@lyubomir-bozhinov

Copy link
Copy Markdown
Owner

DIGEST_CRON can race the Monday 06:00 refresh → a digest built on an incomplete week.

crons.ts: DIGEST_CRON='0 7 * * 1' (Mon 07:00). REFRESH_CRON='0 */6 * * *' fires the Monday 06:00 run at the same instant as PROMPTS_CRON, and the refresh is a durable Workflow that can take minutes on a large window. The digest at 07:00 queries contracts directly, so if the 06:00 refresh is still running, the Monday digest can publish numbers from a partially-refreshed week. Narrow window (steady-state incremental is small) but non-zero on a heavy batch.

Fix: stagger DIGEST_CRON to '0 8 * * 1' (or later) to clear the refresh Workflow's typical completion, or gate the digest on the refresh-complete event rather than a fixed offset.

(The rollups themselves are clean — amount_eur IS NOT NULL throughout weekly.ts, value_suspect excluded from headline spotlights, per-week page noindex, bounded cache. Base-branch merge conflicts need resolving first regardless.)

Review of #80 flagged that the two shim modules used `export * from '@sigma/report'`,
which re-exported the ENTIRE barrel — making ./emit-report-schema and ./report-schema
identical aliases of the whole package and silently hiding any name collision in the
barrel. Replace the wildcards with explicit named re-exports that mirror exactly the
surface each real module (packages/report/src/{emit-report-schema,report-schema}.ts)
always exposed, keeping the module boundaries narrow.

Verified: apps/web typecheck clean under verbatimModuleSyntax (proves every re-exported
symbol resolves in the barrel — export parity — and all import sites still resolve);
@sigma/report 230 tests and @sigma/web 1213 tests pass; @sigma/report already declared
in apps/web dependencies; no dangling references to the removed r2-report-object fixture.
@DiyanaDimitrova

Copy link
Copy Markdown
Collaborator

Благодаря за прегледа — адресирах и четирите точки в 626c27f.

т. 2 (кодова промяна) — стеснен интерфейс. И двата shim-а вече правят експлицитен re-export вместо export *, изброявайки точно символите, които съответният реален модул винаги е излагал:

  • emit-report-schema.tsexport { validateEmitShape, EMIT_REPORT_JSON_SCHEMA, type ShapeResult } from '@sigma/report';
  • report-schema.ts → изричен списък на 8-те стойности (bindReport, sanitizeProse, stripEntityIdPrefix, sanitizeCell, findProseNumbers, asNumber, isImplausibleRatio, MAX_RATIO_MAGNITUDE) и 21-те типа (CellFormat, EntityKind, QueryResult, CellRef, всички Emit*, Resolved*, BindResult, BindOptions).

Така ./emit-report-schema и ./report-schema вече не са псевдоними на целия пакет и една колизия на имена в barrel-а не може да се скрие мълчаливо. (verbatimModuleSyntax налага type-модификаторите.)

т. 1 — паритет на експортите: потвърден. tsc --noEmit на apps/web минава чисто. Под verbatimModuleSyntax това е доказателство, че всеки изброен символ реално се резолвира от barrel-а на @sigma/report (липсващ символ би паднал като „has no exported member"), и че всичките ~30 import места продължават да резолвят с вече стеснените експорти.

т. 3 — декларирана зависимост: потвърдена. @sigma/report: "workspace:*" вече присъства в apps/web/package.json (ред 22).

т. 4 — премахната фикстура: чисто. grep -rn "r2-report-object.fixture" по apps/ и packages/ не намира нито една референция.

Верификация: @sigma/report — 230 теста; @sigma/web — 1213 теста (1 skipped); typecheck и prettier чисти.

…udit

The CI "check" job (osv-scanner) failed: undici 7.28.0 is affected by 5 advisories
(GHSA-4cwx-7wf7-3272 HIGH + four Medium), all fixed in 7.29.0. Raise the undici floor.

This branch carries the security overrides in BOTH pnpm-workspace.yaml and a legacy
package.json `pnpm.overrides` block; the package.json block is the one pnpm actually
applies here (it appears in the lockfile's overrides), so bump it too — as `^7.29.0`
(bounded to the 7.x line, matching #87), not the open `>=7.29.0` selector which pulled
undici 8.x. pnpm-workspace.yaml is set to the same `^7.29.0` + comment, mirroring #87 so
the sibling branches stay in sync for the umbrella merge.

Verified: osv-scanner v2.4.0 `scan source -L pnpm-lock.yaml` → "No issues found" (exit 0);
lockfile diff is undici-only (7.28.0 → 7.29.0); frozen install clean; @sigma/etl 81,
@sigma/web 1213 tests pass.

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified against the branch. The producer/LLM path is genuinely careful — fail-dark gates, constant-time token auth in digest-trigger.ts, zero-row short-circuit with no LLM call or R2 write, and the AI-free fallback keyed on whether narrative text survived verification. Two blockers before this should land:

BLOCKER 1 — is_synthetic header-less contracts are excluded from no digest indicator (accuracy).

  • All eight queries in packages/db/src/queries/weekly.ts filter only amount_eur IS NOT NULL / value_flag='ok', never is_synthetic != 1 (grep: zero is_synthetic in the file) — lines 30, 59, 110, 151, 283, 324, 360, 419.
  • The canonical rollups do filter it: scripts/precompute.sql:64,69,88,92,132 (home_totals/sector_totals/authority_totals/company_totals).
  • Synthetic rows (parent tender is a synthetic неизвестна header, ~11k УНП per 0006_contracts_is_synthetic.sql) can carry a non-NULL amount_eur and title='(без предмет)'. So the digest can (a) over-count total_eur/sector/authority sums vs the rest of the site, (b) surface a '(без предмет)' synthetic as the week's „Най-голяма поръчка", and (c) make getWeeklyCounts.contracts non-zero, bypassing the zero-row publish gate for a week that should not publish.
  • value_flag='ok' is orthogonal — a synthetic row can be value_flag='ok'. And weekly.ts:7 / the reconciliation comment at weekly.ts:505 both claim the week total and home_totals "sum the same amount_eur IS NOT NULL basis" — they don't; home_totals additionally filters is_synthetic, so the "week ≤ home total" tripwire is weaker than documented.
  • weekly.test.ts never inserts an is_synthetic=1 row, so this isn't covered. Fix: add AND c.is_synthetic != 1 to all eight queries, correct the two comments, and add a discriminating test (a synthetic row that would otherwise become the week's largest / inflate a sum).

BLOCKER 2 — migration 0004 prefix collision.

  • The branch's packages/db/migrations/ has both 0004_cpv_division_stats.sql (from base) and 0004_weekly_digests.sql. The base already carries 00000006, so 0004 is taken and the PR description's "assumes #17 lands 0002/0003 first" no longer holds after the base advanced. Renumber to 0007_weekly_digests.sql. (migrations.test.ts references files by name, so CI won't flag the duplicate prefix — this is the cross-PR NNNN-collision trap this repo has hit before.)

Non-blocking: the PR description's pipeline shows persistReport(…, {immutable:true}), but the code intentionally omits immutable (correct — the object is overwritten in place on a re-issue, so immutable cache-control would be a stale-serve trap). Just update the description to match.

Strong work on the auth + producer side — these two are the gate.

Resolve conflicts:
- packages/db/src/queries/index.ts: keep both weekly + related-persons exports
- apps/web/workers/app.integration.test.ts: keep both noindex describe blocks (weeks + conflicts)
- apps/web/app/lib/assistant/describe-schema.ts: keep the @sigma/report shim (#167A);
  carry base's annex_total_suspect value_flag additions into packages/report/src/describe-schema.ts
- osv-scanner.toml / pnpm-workspace.yaml: keep the fuller HEAD suppression docs
- package.json: drop the stale pnpm.overrides block so pnpm 10 reads overrides from
  pnpm-workspace.yaml (base's migration); regenerate pnpm-lock.yaml with pnpm 10.33.0
…o 0007

Resolves both blockers from the review of #80.

BLOCKER 1 (accuracy) — synthetic orphan contracts (parent tender procedure_type=
'неизвестна', title='(без предмет)', is_synthetic=1 per 0006) can carry a non-NULL
amount_eur with value_flag='ok', so the eight weekly.ts indicators were over-counting
sums, could surface a '(без предмет)' row as the week's largest, and could make the
volume count non-zero — bypassing the zero-row publish gate. Add `is_synthetic != 1`
to the shared WEEK_FILTER so all eight queries (and any future one) inherit the guard,
matching precompute.sql's sector/authority/company rollups. Correct the top-of-file and
getWeeklyCounts comments; rewrite the reconciliation comment — home_totals.value_eur is
SUM(amount_eur) over ALL contracts and does NOT itself filter is_synthetic, so the week
is a strict subset of it and 'week ≤ home' is a valid loose bound, not an equal basis
(the review's 'home_totals additionally filters is_synthetic' was inaccurate for that
rollup). Add a discriminating test: a synthetic value_flag='ok' row with the week's
highest amount is excluded from total/largest/top, and a synthetic-only week counts 0.

BLOCKER 2 (migration collision) — the base advanced to 0006, so 0004_weekly_digests.sql
collided with 0004_cpv_division_stats.sql. Renumber to 0007_weekly_digests.sql (git mv;
table name unchanged) and fix the ticket's stale reference.

Also updates the ticket's pipeline note to drop persistReport({immutable:true}), matching
the code (the object is overwritten in place on re-issue, so immutable would stale-serve).

Verified: @sigma/db 350 tests, @sigma/etl 81 tests pass; db+etl typecheck clean; prettier clean.
…ase merge

Merging the advanced base (feat/ai-assistant-contracts) into feat/weekly-digest
renumbered the migration chain — is_synthetic moved to 0012 and the base now reaches
0012 (0007 is taken by 0007_amendment_value_suspect). The weekly_digests migration
(previously renumbered 0004→0007 for the review of #80) collided again, so move it to
0013_weekly_digests.sql. Refresh the two now-stale references: weekly.ts's is_synthetic
provenance comment (0006 → 0012) and the ticket's migration path (0007 → 0013).

No duplicate migration prefixes remain. Verified post-merge: @sigma/db 521 tests,
@sigma/etl 89 tests pass; db typecheck clean; osv-scanner exit 0; check:docs ok.
@DiyanaDimitrova

Copy link
Copy Markdown
Collaborator

Благодаря за прегледа — и двата blocker-а са адресирани (плюс non-blocking-а), на 0aa75ef.

BLOCKER 1 — synthetic контракти в индикаторите (точност). Добавен е is_synthetic != 1 към споделения WEEK_FILTER в weekly.ts, така че и осемте заявки (и всяка бъдеща) го наследяват в едно място — вместо осем отделни редакции, които лесно се разминават. Съответства на филтъра в precompute.sql (sector_totals/authority_totals/company_totals). Поправени са коментарите (заглавен блок + getWeeklyCounts), а reconciliation-коментарът е пренаписан.

Уточнение по reconciliation-а: home_totals.value_eur всъщност е SUM(amount_eur) по всички контракти и не филтрира is_synthetic сам по себе си (precompute.sql ред 115) — така че тезата „home_totals additionally filters is_synthetic" не важи за този rollup. След фикса седмицата е строго подмножество на home_totals (една седмица + без synthetic), значи week ≤ home остава валидна свободна горна граница, но не е сравнение на еднаква база — коментарът вече го описва точно.

Добавен е разграничаващ тест: synthetic ред с value_flag='ok' и най-високата сума в седмицата се изключва от total / largest / top, а седмица само от synthetic редове дава contracts = 0 (zero-row gate-ът не се заобикаля).

BLOCKER 2 — колизия на префикса. След merge на напредналата база (feat/ai-assistant-contracts) веригата се пренареди — is_synthetic вече е 0012, а базата стига до 0012 (0007 е зает от 0007_amendment_value_suspect). Затова weekly_digests миграцията е 0013_weekly_digests.sql (git mv, името на таблицата непроменено). Няма дублирани префикси. Обновени са и двете вече-остарели препратки (comment 0006 → 0012, ticket → 0013).

Non-blocking: описанието на PR-а и ticket-ът вече показват persistReport(…) без immutable — обектът се презаписва на място при re-issue, така че immutable cache-control би бил stale-serve капан.

Проверено: @sigma/db 521 теста, @sigma/etl 89 теста минават; db/etl typecheck чист; osv-scanner exit 0; check:docs ok; prettier чист.

CI 'Coverage ratchet' failed: @sigma/report (new in #167A T1) has a "test" script but
no entry in coverage-baseline.json, and no vitest.config, so it emitted no
coverage-summary.json and check-coverage.mjs fails closed on the missing baseline key.

Add packages/report/vitest.config.ts using the shared sharedCoverage(['src/**']) preset
(mirrors packages/shared) so it emits coverage/coverage-summary.json like every other
workspace, and add its baseline entry (lines 96.3, branches 86.9 — floored from the
measured 96.33/86.93). Verified: full 'pnpm test -- --coverage' + check-coverage.mjs
exits 0, report row 96.33%/86.93%, no workspace below baseline.
@github-actions

Copy link
Copy Markdown

Test coverage

Workspace Lines Δ Branches Δ Functions Statements
apps/etl 85.14% +11.14pp 66.85% +8.65pp 80.61% 84.79%
apps/web 92.08% +1.08pp 83.40% +1.00pp 88.87% 90.22%
packages/config 92.85% +0.05pp 72.22% +0.02pp 92.85% 89.18%
packages/db 94.87% +0.37pp 79.12% -0.18pp 88.38% 91.91%
packages/ingest 89.71% +3.41pp 85.52% +5.12pp 81.74% 88.14%
packages/report 96.33% +0.03pp 86.93% +0.03pp 100.00% 95.21%
packages/shared 95.50% +0.00pp 80.83% +0.03pp 92.30% 89.56%
Total (informational) 92.35% 82.00% 88.58% 90.43%

✅ No workspace dropped below its baseline (tolerance 0.5pp).

📈 Coverage rose by more than 1pp — run node scripts/check-coverage.mjs --update locally and commit coverage-baseline.json to ratchet the threshold up.

Drop docs/tickets/167a-weekly-digest-producer.md and 167b-weekly-digest-consumer.md
(the folder held only these two, so docs/tickets/ goes with them) and their two index
entries in docs/README.md. check:docs stays green — no dangling references remain.
Follow-up to 82ad5c3, which deleted the 167a/167b ticket files but (due to a staging
slip) landed without the docs/README.md edit that removes their two index links. Remove
them now so check:docs has no dangling references.
Drop the midt-bg#167 numeric prefix from the implementation-plan filename; update the single
in-repo reference (docs/README.md). git mv preserves history; check:docs stays green.
Remove .github/workflows/preview.yml and preview-reap.yml — the per-PR `sigma-pr-<n>`
preview-env deploy + reaper. They are fork-only infrastructure (gated to same-repo
branches, keyed to a `preview` GitHub Environment), absent from midt-bg:main and from
the #79 branch that targets it. Dropping them so the upstream PR (midt-bg#324) carries
the same shape as #79 instead of proposing the fork's preview setup to the main repo.
check:docs stays green (the dev-environment docs reference them as inline code, not
validated links).
Review of midt-bg#324 caught a dev-testing config leaked into the committed wrangler.toml
(introduced by 5e760b0 'enable workers.dev route for dev digest-trigger testing'):

- workers_dev = true  → false  (main is false)
- DIGEST_TRIGGER_ENABLED = "true"  → "false"

Neither scripts/wrangler-render.mjs nor deploy.yml rewrites these, so the committed
literals went verbatim to prod: sigma-etl would get a public *.workers.dev URL AND the
on-demand HTTP trigger would be ENABLED, collapsing the three-layer defence (unreachable
+ flag off + bearer token) to just the DIGEST_TRIGGER_TOKEN secret. Both values also
contradicted four in-code comments that state the surface is committed-false / unreachable
(wrangler.toml:7-9, src/index.ts:34 & 297, src/digest-trigger.ts:3).

The trigger opt-in is per-env by design (index.ts:297) — a preview env raises these via
var/render, they are not a committed default. @sigma/etl 89 tests pass; toml valid.
…e refresh boundary

Two findings from the review of midt-bg#324.

MAJOR — deploy.yml never forwarded SIGMA_TURNSTILE_SITE_KEY to the render step. wrangler-
render.mjs reads it (:98) and swaps the per-account key over the committed one (:246) only
when non-empty, but the job env set just SIGMA_BUILD_ID/ASSISTANT_ENABLED/ENVIRONMENT/
AI_GATEWAY_ACCOUNT — so the value was always '', the swap a no-op, and the committed prod
(domain-bound) Turnstile SITE key shipped to every environment. The moment a non-prod env
turns on the assistant (SIGMA_ASSISTANT_ENABLED=true), the prod key can't validate on that
domain and the bot-gate (useTurnstileGate) silently breaks. Add
SIGMA_TURNSTILE_SITE_KEY: ${{ vars.SIGMA_TURNSTILE_SITE_KEY }} alongside the other four
(same missing-var class as SIGMA_AI_GATEWAY_ACCOUNT). Verified via render dry-run: set →
dev key renders, unset → committed prod key stays.

MINOR — PROMPTS_CRON '0 6 * * 1' fired at exactly the Monday 06:00 REFRESH_CRON slot, so
scheduled() ran twice and generateSuggestedPrompts raced itself on the same D1 (idempotent
per-slot upserts, so wasted compute + last-write-wins refreshed_at, not corruption). Since
the 6-hourly refresh already regenerates prompts and PROMPTS_CRON is only the coarse weekly
fallback, move it to '5 6 * * 1' — runs just AFTER the Monday refresh (what a fallback
should do) and clears every refresh slot (:00). wrangler.toml [triggers].crons updated in
lockstep (cron-guard test enforces crons.ts == wrangler.toml).

@sigma/etl 89 tests pass (incl. cron-guard); deploy.yml valid; render swap verified.
lyubomir-bozhinov pushed a commit that referenced this pull request Aug 26, 2026
)

* test(ci): gate the fake-D1 doubles, and fail on an unmatched query

midt-bg#325: 24 test files hold 36 `as D1Database` casts, one hand-rolled double each.
Every one dispatches on `sql.includes('…')` and falls through to `{ results: [] }`
when no marker matches, so renaming a CTE or reordering a JOIN leaves the test
green against emptiness — asserting nothing. Only details.test.ts throws today.

This is the acceptance test for that work, written before the work: outside an
explicit allowlist, no file under apps/ or packages/ may type a value as a
D1Database. It is red now (24 files, 36 casts) and goes green when the last
double moves to the shared helper.

The allowlist is by name, never a directory glob — the argument the midt-bg#254 review
already made about the coverage exclusion list. A glob lets a new double leave
the gate by where it sits; a named entry means someone had to add it, which is
reviewable. A stale entry is an error rather than a no-op, so a renamed double
cannot leave the gate widened by a line nobody reads again. That fail-closed
branch is what fires right now, since the helper does not exist yet.

Matching is over blanked source — comments, strings and regex literals removed,
byte positions kept — so a comment describing the old design is not a finding.
`as unknown as D1Database` is matched before `as D1Database` because the short
spelling is a suffix of the long one and a naive pattern counts one cast twice.
`satisfies` is covered too: it is the only other operator that types a literal.

Self-test is mutation-checked — dropping the `as unknown as` alternative, the
`satisfies` alternative, the comment blanking, the trailing word boundary, or
the stale-entry check each kills exactly one named test, and no others.

scripts-test.yml needs no edit: its lane already globs scripts/*.test.mjs.

* test(test-support): a shared D1 double that throws on an unmatched query

The helper midt-bg#325 asks for, as its own private workspace. `@sigma/db` exports only
`.`, and neither apps/etl nor packages/ingest depends on it, so putting the
double under db/src/test/ would have meant a subpath export plus two new
workspace deps. A separate package also sits outside all six measured
workspaces, so it cannot enter their coverage denominators by construction —
stronger than the by-name vitest.shared.ts exclusion the issue proposes, and it
leaves that file (which midt-bg#254 rewrites) untouched.

A route is a marker set and a response; every marker must appear in the SQL, and
the first matching route wins so a specific route can precede a general one.
Unmatched throws, naming the offending statement and every registered marker.
`{ onUnmatched: 'empty' }` buys emptiness back, at the call site, in writing.

Three entry points, one core: fakeD1 for query tests, recordingD1 for the tests
of a *wrapper* over D1 (readonlyD1) that must accept arbitrary SQL and assert on
a call log, throwingD1 for the error paths.

Two design notes worth keeping:

  - `first` is typed `object | null | (call) => object | null`, not `unknown`.
    A top type absorbs the union and the callback form silently loses its
    parameter type — tsc caught it. A D1 row is an object or nothing anyway.
  - No pagination feature. Keyset slicing is already `all: (call) =>
    rows.filter(r => r.id > call.binds.at(-2))`, which is what the doubles in
    companies.test.ts do by hand today.

Tests written before the code, behaviour by behaviour, and mutation-checked:
never throwing on an unmatched all() or first(), matching a route that answers a
different method, `some` for `every` over the markers, last-match instead of
first, no truncation, dropping the marker list from the message, discarding
bind() arguments, not recording prepare() or batch(), and ignoring throwingD1's
supplied error — eleven mutations, each killing a specific named test.

The `?? []` fallback in all() went away rather than getting a test: the route
lookup already guarantees the response is defined, so it was unreachable.
Returning the response instead of the route also keeps `first: null` — a route
meaning "no such row" — distinct from no route at all.

Seventh key in coverage-baseline.json: check-coverage's findTestWorkspaces fails
closed on a workspace that has a test script without a baseline entry, and this
one should be measured. 100% lines, 100% branches. The six existing workspaces
are untouched — they were already reading above their baselines before this
branch, which is pre-existing drift and not for a test refactor to ratchet.

* test(test-support): keep `sql` live, and fail the throwing double at execution

Two defects the first migration batch walked straight into.

`sql` was a getter over `calls`, so `const { db, sql } = fake()` — the natural
way to use it, and what flows.test.ts and authorities.test.ts already wrote
against their hand-rolled spies — captured an empty snapshot that never filled
in. Every later assertion then read nothing and passed for the wrong reason,
which is the exact failure this helper exists to remove. It is a live array kept
in step with `calls` now, and a test pins the destructured form.

throwingD1 threw from prepare(). D1's prepare() is lazy and never touches the
database: a missing table surfaces on all()/first()/run(). A double that failed
earlier would let a test claim it covers an error path it never reaches — and
related-persons.test.ts, whose whole point is that an un-migrated environment
degrades instead of 500ing, hand-rolled a double that threw at execution for
exactly that reason. It now rejects from the three execution methods and records
the statement that failed, so the offending SQL stays inspectable.

* test(db): route the query doubles through the shared fake

Sixteen files in packages/db/src/queries, each of which built its own D1 double
that dispatched on `sql.includes('…')` and fell through to no rows. Fixtures and
assertions are unchanged — only the double moves.

Measured before touching anything, by breaking each marker in the production SQL
and running the test: ELEVEN marker paths across nine files stayed green against
an emptied result. authorities (FROM authority_totals), companies (ORDER BY
bidder_id), competition and trend and flows (FROM sector_totals), contracts
(facet_counts), home (bids_received = 1, JOIN), network (FROM company_totals,
FROM authority_totals WHERE authority_id), search (sqlite_master). Every one of
them now rejects with the marker set it was looking for.

Three things the migration turned up that were not in the issue:

  - regions.test.ts served *region* rows to sectorOptions, which asks a
    completely different table. It reached the same answer only because
    sectorOptions reads r.division, the region fixture has no such field, and
    the filter dropped every row. The route says `all: []` now, and says why.
  - companies.test.ts registered two facet routes for queries no test in it ever
    issues — getCompanyFacets is not exercised there. Dropped rather than kept
    as decoration.
  - companies' CSV stream and list query both read company_totals, so breaking
    the stream's ORDER BY quietly fell through to the list route and returned an
    unpaginated page. They are separated by their own markers now (ORDER BY
    bidder_id vs AS sort_value), and breaking either one throws.

Two markers still survive being broken — authorities' and companies' `FROM
<rollup>`. That is the harness, not the tests: `FROM ${src.from}` is composed at
runtime, so the literal never appears in the source to be mutated. Mutating the
`from:` value itself is caught by both.

Route matching is still substring-based, so a query can fall from a specific
route to a more general one in the same set. What is gone is the *default*
fall-through to emptiness — an unrouted query throws.

packages/db: 487 tests pass; coverage unmoved.

* test(test-support): one real-SQLite D1 facade instead of four copies

d1FromSqlite lived in packages/ingest/src/test/, and packages/db had two
byte-identical re-implementations of it (contracts-filter-sql, value-base-sql,
differing only in a local variable name) while apps/etl reached the original
through ../../../packages/ingest/src/test/d1-sqlite — a relative path across a
workspace boundary, which is what a missing shared home looks like.

It moves to @sigma/test-support beside the fake. The two are different tools and
stay different: this one runs the real SQL against a real node:sqlite database
where SQL semantics are what is under test; fakeD1 is for the TypeScript logic
around a query. Now they at least live in the same place, and the gate's
allowlist names one package instead of two.

Slightly wider than midt-bg#325 asked — the issue scopes itself to the fake doubles and
puts real-SQLite tests out of scope. It is here because "one cast everywhere" is
one of its own done-when boxes, and two of the four remaining casts were these
copies. Reviewer's call; it lifts out cleanly.

Side effect worth noting: d1-sqlite.ts leaves packages/ingest's coverage
denominator by leaving the workspace, which is the outcome midt-bg#254 wanted from a
by-name exclusion, reached by construction instead.

db 487, ingest 84, etl 20 — all pass.

* test(db): recording doubles for the two readonly wrapper suites

readonly-d1 and readonly-corpus test a *wrapper* over D1, not a query: what
matters is which statements reach the handle underneath, not what comes back.
Marker dispatch is the wrong shape for that, so both use recordingD1 — answers
anything, records everything — with `when: []`, a route that constrains nothing.

Two things came out of it, both in the helper:

  - `when: []` matching every query was already true (every() over no markers),
    but undocumented and unpinned. Now both.
  - readonly-d1's hand-rolled log tagged its entries `prepare:` / `exec:`, and
    flattening that into plain SQL would have cost the test its point: a wrapper
    that sent an exec down the prepare path emits identical text, and the
    assertion could no longer tell. FakeD1Call carries `via` now, and the
    corpus's zero-proxy row survives as the response to a constraint-free route.

readonly-corpus also dropped a `raw()` no production path calls.

packages/db: 487 tests pass.

* test(etl): route the ETL doubles through the shared fake

Three doubles. The integrity gate's fake dispatched on eleven markers and fell
through to no rows; it now names all eleven as routes and rejects anything else.
Its local builder was called `fakeD1`, which is the shared helper's name, so it
becomes `servedD1` — which is what it models anyway: a served D1 after
precompute, not any old one.

eop.test.ts also passed `{} as D1Database` twice, for paths that fail before
they reach the database. `fakeD1([])` states that instead of implying it: a
route-less double rejects any query, so if one of those paths ever did reach D1
the test would say so rather than throwing an incidental TypeError on an empty
object.

The freshness double's guard survives as a route that throws its own message —
"raw staging should not be read for planning" is a claim worth keeping in the
test, rather than degrading to the generic no-route error.

apps/etl: 20 tests pass.

* test(test-support): per-route meta, and cover the SQLite facade in its new home

apps/web's assistant tests need meta.rows_read and meta.total_attempts: they
drive the rows-read budget that keeps a retried full scan from under-billing the
Denial-of-Wallet limit (midt-bg#122, review #80). Flattening that to a fixed empty meta
would have quietly removed what those two tests assert, so a route can declare
its own meta. Default stays `{}`.

Moving d1-sqlite.ts here left it with no tests of its own — its callers live in
db, ingest and etl, and none of them count toward this workspace. The ratchet
caught it at 81% and it is covered directly now, including the case nothing
tested anywhere before: batch() rolls back when one statement fails. A
half-applied batch would leave a fixture in a state no production path can
reach, and whoever met it would be debugging a ghost.

While covering it, throwingD1's bind() read `calls.at(-1)` — so binding statement
A after preparing B recorded the arguments against B. Same statement-independence
bug fakeD1 already had a test against; it captures its own record now, and so
does the test.

100% lines, 100% branches, 44 tests.

* test(web): route the last two doubles through the shared fake

assistant/tools built a double whose only real job was carrying meta; it now
declares that meta on a route. csv-export asserted the expected SQL *inside* its
fake — that assertion becomes the route's own marker, so a query that no longer
matches rejects and names both the statement and what was expected, instead of
failing an inline expect from inside a stub.

With these two the gate from the first commit goes green: 279 files scanned, no
D1Database cast outside @sigma/test-support. It opened at 36 casts in 24 files.

apps/web: 493 tests pass.

* fix(test-support): route exec() and batch() through the same contract

batch() recorded each statement and returned a synthetic success without ever
consulting the routes, so a batch of unregistered SQL passed against nothing —
the silent green this helper exists to kill, on the one entry point the write
paths use exclusively (staging, refresh, fx never call prepare().run()).
exec() had the identical hole one method up.

Both now look the statement up. They ask only whether it is registered at all,
not for a particular response shape the way all()/first()/run() do, and throw
naming the SQL and every marker when it is not.

Also: run() and batch() carry the `results` key a real D1Result always has —
the cast to D1Database was hiding its absence; the header no longer points at
the facade's pre-move path; and the second batch() record is documented as a
log of entry points rather than a double count.

* fix(test-support): carry the full D1Result shape in the SQLite facade

all() returned no `meta`, run() neither `meta` nor `results`, batch() no
`results`. The cast to D1Database hid every one of them: the first caller to
read one would get `undefined` from the facade where real D1 hands back `[]`
or `{}`. One D1Shape type spells out all three keys.

* test(web): keep the exact-SQL check csv-export had before the migration

The hand-rolled double asserted `expect(sql).toBe(...)` on the whole statement.
Migrating turned that string into a `when` marker, and markers match by
substring — so the one place the refactor loosened a check rather than
tightening it. Measured: wrapping the production statement leaves all 34 tests
green. The equality moves inside the route, where the callback sees `call.sql`.

* test(ci): pin the gate's scan roots and catch an aliased D1Database

Two ways past the gate, both reproduced. `type DBAlias = D1Database` and then
`as unknown as DBAlias` leaves no D1Database token for the pattern to find; a
renamed type import does the same. A second pass treats giving the type another
name outside the allowlist as the offence, while leaving ordinary annotations
(`db: D1Database`, a field on an Env type) alone.

And SCAN_ROOTS was module-private, so deleting 'apps' from it left the self-test
12/12 green while web and etl dropped out of enforcement. Exported and pinned: a
pattern applied to half the repo is a gate that passes while enforcing nothing.

* fix(test-support): route a batched statement by what it does, not its markers

Markers alone are blind to the method, and it is measurable: a route declaring
only `all:` answered a batched `DELETE FROM staging` with its rows, because
`FROM staging` is a substring of the write. The same SQL through prepare().run()
threw. Narrower than an unrouted batch, but the same silent pass.

A batched write now needs a `run:` route and a batched SELECT an `all:` one;
neither settles for the other, and a SELECT no longer fires a write effect it
happens to match. A write still serves rows when it has them, for RETURNING.
exec() asks for `run:` too — it hands back no rows, so nothing else means
anything to it. That retires `registered()`: every entry point is method-aware.

Reading is decided by the leading keyword, so a `WITH … INSERT` reads as a
SELECT here. That costs a false rejection, never a false pass.

Also: run() carries the route's meta, which batch() already resolved for the
same statement, and batch() documents that it is not transactional — real D1
and the d1-sqlite.ts facade roll back, this does not.

* test(ci): widen the alias rule to where an alias actually goes

The pattern closed three spellings while the comment promised the class. Four
more walked past it: `D1Database & {}`, `Pick<D1Database, …>`, a namespaced
`import('…').D1Database`, and a heritage list naming it off the first position.

The rule is now positional — the mention must sit right of `=`, or inside an
intersection, union, type argument or namespace, never where a parameter or a
field goes. `type Env = { DB: D1Database }` and the conditional type in
readonly-corpus.test.ts stay clean, both pinned.

`implements` is deliberately out: ReadonlyD1 implements D1Database in
production, and TypeScript forces a complete implementation there, so it is no
shortcut to a stub. The comment now says best-effort and means it.

---------

Co-authored-by: Todor Kolev <tkolev@obecto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants